feat(server) : snapshot traverse physical buckets #4084
Conversation
Signed-off-by: adi_holden <[email protected]>
src/core/dash.h
Outdated
| if (curs.bucket_id() >= SegmentType::kTotalBuckets) // sanity. | ||
| return 0; |
There was a problem hiding this comment.
Wouldn't this restart the main loop of the caller since 0 means to start again and probably we ended up in this situation because of what re returned below ? Why not a DCHECK ? (I know it's WIP I just saw this and thought to ask)
There was a problem hiding this comment.
returning 0 means iteration finished, we have the same logic in Traverse function
Signed-off-by: adi_holden <[email protected]>
adiholden
changed the title
src/core/dash.h
Outdated
| // Takes an iterator pointing to an entry in a dash bucket and traverses all bucket's entries by | ||
| // calling cb(iterator) for every non-empty slot. The iteration goes over a physical bucket. | ||
| template <typename Cb> void TraverseBucket(const_iterator it, Cb&& cb); | ||
| // Traverses over a single physical bucket in table call cb once on bucket iterator. |
There was a problem hiding this comment.
| // Traverses over a single physical bucket in table call cb once on bucket iterator. | |
| // Traverses over physical buckets. It calls cb once for each bucket by passing a bucket iterator. |
src/core/dash.h
Outdated
| uint32_t sid = curs.segment_id(global_depth_); | ||
| uint8_t bid = curs.bucket_id(); | ||
|
|
||
| bool fetched = false; |
There was a problem hiding this comment.
Nit: While it's not strictly necessary to iterate until we find a non-empty bucket, we could potentially iterate over the entire empty table without returning. Conversely, for densely packed tables, we might return after checking each bucket. To balance these scenarios, iterating over a fixed number of buckets (e.g., 8 or 16) regardless of the number of non-empty buckets encountered might be a smoother approach.
| @@ -566,26 +566,6 @@ TEST_F(DashTest, Traverse) { | |||
| EXPECT_EQ(kNumItems - 1, nums.back()); | |||
| } | |||
|
|
|||
| TEST_F(DashTest, Bucket) { | |||
There was a problem hiding this comment.
don't you want to test the change?
Signed-off-by: adi_holden <[email protected]>
Signed-off-by: adi_holden <[email protected]>
Signed-off-by: adi_holden <[email protected]>
|
🚢 🇮🇹 |
This PR introduces new api to dash table TraversingBuckets which traveses table by physical buckets
when creating a snapshot we now call this api. The snapshot correctness stays and there are less calls to Serialize bucket callback as it is called only once per bucket and not on every slot